Design improvement for scaling win32 widgets #62 #2483
Conversation
This commit improves thes design implementation for widgets in win32 by moving away from DPIZoomChangeRegistry to an event-driven design using ZoomChanged event and inheritance. contributes to eclipse-platform#62 and eclipse-platform#128
There was a problem hiding this comment.
I am a bit confused by this change, as it introduces a mixture of event processing and direct method calls for DPI change processing. I expected the following order of changes according to what we have discussed:
- Replace
CommonWidgetsDPIChangeHandler: use listeners instead - Replace
DPIZoomChangeRegistry: make direct calls ofhandleDPIChangefor the native controls instead - Replace direct
handleDPIChangecalls: use listeners instead - Make the listener calls asynchronous (may be combined with the previous step)
This change seems to be a mixture of the first three. Can we please split that up and make the changes in a more incremental, comprehensive way?
| for (TreeItem item : treeItem.getItems()) { | ||
| DPIZoomChangeRegistry.applyChange(item, newZoom, scalingFactor); | ||
| for (TreeItem item : getItems()) { | ||
| item.notifyListeners(SWT.ZoomChanged, event); |
There was a problem hiding this comment.
Shouldn't this be a directcall to handleDPIChange for now?
| @@ -191,6 +190,14 @@ public Widget (Widget parent, int style) { | |||
| reskinWidget (); | |||
| notifyCreationTracker(); | |||
| this.setData(DATA_NATIVE_ZOOM, this.nativeZoom); | |||
| registerDPIChangeListener(); | |||
There was a problem hiding this comment.
With this PR, we stick to the synchronous processing of the event via direct methods calls, so no need to aready add this listener, right?
| private void handleCComboDPIChange(Event event) { | ||
| updateAndRefreshChildren(childWidget -> { | ||
| childWidget.notifyListeners(SWT.ZoomChanged, event); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This intermediate method can be removed by directly making the notifications in the update method.
| addListener(SWT.ZoomChanged, event -> { | ||
| handleCComboDPIChange(event); | ||
| }); |
There was a problem hiding this comment.
| addListener(SWT.ZoomChanged, event -> { | |
| handleCComboDPIChange(event); | |
| }); | |
| addListener(SWT.ZoomChanged, this::handleCComboDPIChange); |
| @@ -2026,19 +2029,24 @@ public boolean traverse(int event){ | |||
| return super.traverse(event); | |||
| } | |||
|
|
|||
| private void handleCComboDPIChange(Event event) { | |||
There was a problem hiding this comment.
| private void handleCComboDPIChange(Event event) { | |
| private void handleDPIChange(Event event) { |
| styledText.setCaretLocations(); | ||
|
|
||
| caretSet.stream().filter(Objects::nonNull).forEach(caretToRefresh -> { | ||
| ((Caret) caretToRefresh).notifyListeners(SWT.ZoomChanged, event); |
There was a problem hiding this comment.
The set is already types as Caret, so why cast to Caret here again?
| } | ||
| composite.redrawInPixels (null, true); | ||
| // redrawInPixels (null, true); |
| private static void handleDPIChange(Widget widget, int newZoom, float scalingFactor) { | ||
| widget.nativeZoom = newZoom; | ||
| widget.setData(DATA_NATIVE_ZOOM, newZoom); | ||
| void handleDPIChange(Event event, float scalingFactor) { |
There was a problem hiding this comment.
Why change the signature here instead of sticking to the existing parameters?
| for (int i = 0; i < tabFolder.getItemCount(); i++) { | ||
| DPIZoomChangeRegistry.applyChange(tabFolder.items[i], newZoom, scalingFactor); | ||
| for (int i = 0; i < getItemCount(); i++) { | ||
| items[i].notifyListeners(SWT.ZoomChanged, event); |
There was a problem hiding this comment.
Shouldn't this be items[i].handleDPIChange(...) for now?
This PR improves thes design implementation for widgets in win32 by moving away from DPIZoomChangeRegistry to an event-driven design using ZoomChanged event and inheritance.
contributes to
#62 and #128